feat: re-write uuid functions (calls + DEFAULT) in omni insert - #1556
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
e5c7b00 to
35e6e1c
Compare
35e6e1c to
1d6511c
Compare
| /// Why do we need to cast? This is because, for example, if we try to use CURRENT_TIME (timetz) with a | ||
| /// text column in a binary format Bind param, Postgres will error without an explicit cast (expected UTF-8). | ||
| /// The alternative is manually calculating the binary format, which is a lot more of a headache! :) |
There was a problem hiding this comment.
I don't think this comment is correct. We don't send OIDs when rewriting the parse message, so it'll always be an inferred type depending on the position. If it's in a ResTarget on the values list for an insert or update, it'll be inferred to the type of the column, while in all other cases it'll almost certainly be inferred to text.
None of this should be necessary if we start sending the appropriate types in our rewritten Parse messages.
There was a problem hiding this comment.
Interesting. This wasn't my first attempt; I came to this workaround after I was unable to get it to work without any special treatment. Postgres was throwing errors with the extended protocol in relation to how the binary format was assembled in the Bind message. There's certainly a chance I was doing something wrong, missing something, or perhaps there's a bug somewhere.
There was a problem hiding this comment.
Right, bind parameters of the incorrect type are far more likely to work incidentally over the text protocol, since the same string can be a valid value across multiple types, and the fallback type for a parameter when one can't be inferred is text (casting a text parameter to another type is the same as sending a value of that type using the text protocol)
The type of a bind parameter is only going to be inferrable some of the time. (inserts and updates for sure, selects will depend on context). When it can't be inferred, it'll fall back to text. The correct binary repr of a time/timestamp is unlikely to be valid UTF-8 so it'll cause a decoding error when that happens. Sending the OID means that PG will always know we intended to send the timestamptz, so sending the binary repr (which we should prefer in this case because dealing with text representations of dates/times is much more expensive than something like an integer) will always be interpreted the correct way
There was a problem hiding this comment.
I see, thank you for explaining. I can work on fixing this as part of my WIP refactor for supporting SELECT rewrites for functions that rely on transaction time (#1624)
There was a problem hiding this comment.
Sure, but we should touch base to make sure we don't step on each other's toes. I'm doing a pretty significant refactor of this part of the code base right now
3 participants
Adds support to rewrite UUID functions (
uuidv4,uuidv7,gen_random_uuid) for omni inserts on top of the support for date-time functions (from #1541). Some refactoring to push those two classes of functions into separate files and generalize handling a bit more.It does not support specifying an interval argument within uuidv7() yet.